DHCP: implement RFC 3396 encoding of long options (fixes #4642)#4950
DHCP: implement RFC 3396 encoding of long options (fixes #4642)#4950StrugglingKeyboard wants to merge 1 commit intosecdev:masterfrom
Conversation
Split DHCP options longer than 255 bytes into multiple consecutive TLV entries during serialization (i2m), and concatenate consecutive options with the same code during dissection (m2i), as specified by RFC 3396. Also add support for option overload (code 52) in getfield, building the aggregate option buffer from the options, file and sname fields as described in RFC 3396 section 5.
There was a problem hiding this comment.
I think it's really cool that
DHCP(options=[('captive-portal', b'a'*256)])can automatically split the option. That being said I'm not sure how safe it is to start concatenating the options by default. Personally in some cases I think it would still be better to be able to see all the options (and the existing scripts actually expect that).
The commit message should probably point to #4343 as well.
(The tests fail because zero-length options get lost
>>> raw(DHCP(options=[('rapid_commit', b'')]))
b''It should be
>>> raw(DHCP(options=[('rapid_commit', b'')]))
b'P\x00'Those tests do pass with the master branch)
| assert DHCP(b'\x06\x02\x01\x02\x06\x02\x03\x04').options == DHCP(b'\x06\x04\x01\x02\x03\x04').options | ||
|
|
||
| p = DHCP(b'\x0c\x02sc\x06\x04\x01\x02\x03\x04\x0c\x02py') | ||
| assert p.options == [('hostname', b'sc'), ('name_server', '1.2.3.4'), ('hostname', b'py')] |
There was a problem hiding this comment.
I don't think it's correct. The RFC says
When a decoding agent is scanning an incoming DHCP packet's option
buffer and finds two or more options with the same option code, it
MUST consider them to be split portions of an option as described in
the preceding section.
and the preceding section says
The
split portions of the option MUST be stored in the aggregate option
buffer in sequential order - the first split portion MUST be stored
first in the aggregate option buffer, then the second portion, and so
on.
so the Host Name option should be scpy in this particular case and that's what wireshark shows for example
Option: (12) Host Name
Length: 2
Encoding Long Options detected (RFC 3396): 1/2
[Expert Info (Chat/Reassemble): For the data, please refer to the last option 12, 2/2]
[For the data, please refer to the last option 12, 2/2]
[Severity level: Chat]
[Group: Reassemble]
Option: (6) Domain Name Server
Length: 4
Domain Name Server: 1.2.3.4
Option: (12) Host Name
Length: 2
Encoding Long Options detected (RFC 3396): 2/2
Host Name: scpyThere was a problem hiding this comment.
Thanks a lot for the thorough review! You're right, I misinterpreted the concatenation part, and your Wireshark example makes it very clear. I've also blown up the regression test on zero-length options without realizing it. Really sorry about that.
Regarding the concern about backward compatibility: I think making the concatenation opt-in through conf.contribs["DHCP"]["rfc3396"] defaulting to False would be the safest approach then. That way existing scripts remain unaffected, and users who need RFC 3396 compliance can enable it explicitly.
Does that sound reasonable? I'll push an updated version in the coming days.
There was a problem hiding this comment.
Does that sound reasonable?
I'm not a scapy maintainer so I think it would make sense to wait for the scapy maintainers to figure out what the right direction is. It's just that I have use cases where I'm more interested in raw options so from my perspective it makes sense to keep it working one way or another but then again it's not up to me.
2 participants
Description
Implements RFC 3396 (Encoding Long Options in DHCPv4) in Scapy's DHCP layer.
Fixes #4642
Problem
raw(DHCP(options=[('captive-portal', 'a'*256)]))was raisingstruct.error: ubyte format requires 0 <= number <= 255because the option length field is encoded as a single unsigned byte, limiting option values to 255 bytes. Similarly, receiving a DHCP packet containing consecutive options with the same code (as produced by RFC 3396 encoding) results in incorrect dissection.Changes
All changes are in
scapy/layers/dhcp.py, within the classDHCPOptionsField:i2m: options with values exceeding 255 bytes are automatically split into consecutive TLV fragments of at most 255 bytes each, all carrying the same option code._concat_fragments(new): helper method that concatenates consecutive raw fragments with the same option code.m2i: uses_concat_fragmentsto reassemble fragmented options before interpretation, for both known and unknown option codes._find_overload(new): scans the raw options buffer for optionoverload(code 52).getfield: when optionoverloadis present, builds the aggregate option buffer from the options, file and sname fields (RFC 3396 section 5) before passing it tom2i.Tests
Added a test covering: encoding (split at 256 bytes), decoding (fragment concatenation for typed fields, non-concatenation of non-consecutive fragments, unknown option codes), roundtrip (encode then decode), and option overload (negative and positive cases).
Note: Two pre-existing test failures in
test/scapy/layers/dhcp.uts(tests "DHCP - build" and "DHCP - dissection", both on the s5/p5 packet) are unrelated to this change and also fail on the current master branch.